Support MemProf on darwin #69640
Support MemProf on darwin #69640
Conversation
llvmbot
added
clang
You can test this locally with the following command:git-clang-format --diff ea9e116e5a24e834142bc4024b57697b48599a9e 8ce32592e780e693a9378f30237a25629a822897 -- clang/test/Driver/darwin-memprof.c compiler-rt/lib/memprof/memprof_mac.cpp compiler-rt/lib/memprof/memprof_malloc_mac.cpp compiler-rt/test/memprof/TestCases/Darwin/memprof_dump.cpp clang/lib/Driver/ToolChains/Darwin.cpp compiler-rt/lib/memprof/memprof_allocator.cpp compiler-rt/lib/memprof/memprof_allocator.h compiler-rt/lib/memprof/memprof_interceptors.cpp compiler-rt/lib/memprof/memprof_interceptors.h compiler-rt/lib/memprof/memprof_linux.cpp compiler-rt/lib/memprof/memprof_malloc_linux.cpp compiler-rt/lib/memprof/memprof_new_delete.cpp compiler-rt/test/asan/TestCases/Darwin/asan-symbolize-templated-cxx.cppView the diff from clang-format here.diff --git a/compiler-rt/lib/memprof/memprof_mac.cpp b/compiler-rt/lib/memprof/memprof_mac.cpp
index 71439501bb11..a931dd364b0b 100644
--- a/compiler-rt/lib/memprof/memprof_mac.cpp
+++ b/compiler-rt/lib/memprof/memprof_mac.cpp
@@ -42,9 +42,7 @@ void InitializePlatformInterceptors() {}
void InitializePlatformExceptionHandlers() {}
// No-op. Mac does not support static linkage anyway.
-void *MemprofDoesNotSupportStaticLinkage() {
- return 0;
-}
+void *MemprofDoesNotSupportStaticLinkage() { return 0; }
uptr FindDynamicShadowStart() {
uptr shadow_size_bytes = MemToShadowSize(kHighMemEnd);
|
There was a problem hiding this comment.
Thanks for sending the patch! I took an initial look through and have some comments/questions.
@snehasish may also be interested in taking a look.
| else() | ||
| set(COMPILER_RT_ASAN_HAS_STATIC_RUNTIME FALSE) | ||
| set(COMPILER_RT_MEMPROF_HAS_STATIC_RUNTIME TRUE) |
There was a problem hiding this comment.
Is this meant to be FALSE? Otherwise this new variable is always TRUE.
| @@ -78,7 +78,11 @@ static int GetCpuId(void) { | |||
| // will seg fault as the address of __vdso_getcpu will be null. | |||
| if (!memprof_inited) | |||
| return -1; | |||
| #if SANITIZER_APPLE | |||
| return 0; | |||
There was a problem hiding this comment.
If there is a way to do this on Apple then add a FIXME?
| ArrayRef<LoadedModule> Modules(List.begin(), List.end()); | ||
| for (const auto &Module : Modules) { | ||
| for (const auto &Segment : Module.ranges()) { | ||
| if (true) { // Segment.executable) { |
There was a problem hiding this comment.
Should the if-condition be always true? Leftover debugging handling?
| for (const auto &Segment : Module.ranges()) { | ||
| if (true) { // Segment.executable) { | ||
| SegmentEntry Entry(Segment.beg, Segment.end, Module.base_address()); | ||
| // CHECK(Module.uuid_size() <= MEMPROF_BUILDID_MAX_SIZE); |
There was a problem hiding this comment.
Should this check and the line below it be uncommented?
| void FinishAndWrite() { | ||
| if (print_text && common_flags()->print_module_map) | ||
| DumpProcessMap(); | ||
|
|
||
| allocator.ForceLock(); | ||
|
|
||
| InsertLiveBlocks(); | ||
| #if SANITIZER_APPLE |
There was a problem hiding this comment.
Which part of this handling is Apple-specific?
| B *b = new B; | ||
| test_2(b); | ||
| __sanitizer_set_report_path("stdout"); | ||
| //__memprof_profile_dump(); |
There was a problem hiding this comment.
Are these commented out lines supposed to be here?
| } | ||
|
|
||
| void test_5(C *p, void (C::*q)(void)) { | ||
| // We also use type.checked.load for the virtual side of member function |
There was a problem hiding this comment.
I'm confused by all the comments about type.checked.load - this isn't being built with the necessary options.
| // CHECK: __ZTV1B | ||
| // 5 sections corresponding to xxx xxx __got __mod_init_func __const | ||
| // vtables are in __const | ||
| // CHECK: BuildIdName:{{.*}}memprof-vtable |
There was a problem hiding this comment.
Where is the memprof-vtable module name coming from?
| @@ -63,6 +57,23 @@ struct AP64 { // Allocator64 parameters. Deliberately using a short name. | |||
| template <typename AddressSpaceView> | |||
| using PrimaryAllocatorASVT = SizeClassAllocator64<AP64<AddressSpaceView>>; | |||
| using PrimaryAllocator = PrimaryAllocatorASVT<LocalAddressSpaceView>; | |||
| #endif | |||
There was a problem hiding this comment.
Combine these 2 lines into an "#else" ?
| @@ -43,6 +43,7 @@ enum class align_val_t : size_t {}; | |||
| ReportOutOfMemory(size, &stack); \ | |||
| return res; | |||
|
|
|||
| #if !SANITIZER_APPLE | |||
There was a problem hiding this comment.
How do operator new and delete get intercepted on Apple?
Also, are OPERATOR_NEW_BODY* and OPERATOR_DELETE_BODY* macros needed if these interceptors are not included - can the whole thing be put under "#if !SANITIZER_APPLE"?
There was a problem hiding this comment.
It would be nice to split this patch into at least two parts:
- Add basic support for Darwin (malloc implementation, build ids, raw format etc)
- Record and dump references to static data in the binary
I think there are a number of places where we assume x86-ELF and I would like to make sure we address any issues before adding more functionality.
Memory profiler is supported on Linux currently and we are interested in enabling it on Darwin. There is existing Darwin support for ASan, supporting memory profiler on Darwin should be similar to Darwin Support in ASan. On top of the goals such as tooling, guiding memory allocator, etc, we would like to use it to dump information around accesses to the binary itself. This information can help us understand the page faults triggered during app startup and hopefully be used in optimizing the binary layout to reduce page faults.